Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eventual BG with 1 decimal in the app and in the watch app #320

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

bjornoleh
Copy link
Contributor

@bjornoleh bjornoleh commented Jun 17, 2024

  • Use targetFormatter instead of numberFormater for eventualBG in the app, since this has the appropriate maximumFractionDigits = 1

  • Change eventualFormatter to use maximumFractionDigits = 1 instead of 2 on the watch

Resolves #319

- Use `targetFormatter` instead of `numberFormater` for `eventualBG` in the app, since this has the appropriate `maximumFractionDigits = 1`

- Change eventualFormatter to use `maximumFractionDigits = 1` instead of `2` on the watch
@bjornoleh
Copy link
Contributor Author

Eventual BG as presented with this patch:

52,8 mmol/L to the left, 951 mg/dL to the right (random numbers from sim phone/watch/CGM and pump)
image

@mountrcg
Copy link
Contributor

@bjornoleh there is a glucoseformatter also in ./FreeAPS/Sources/Modules/Home/View/Header/CurrentGlucoseView.swift which should fit perfectly for this issue

@bjornoleh
Copy link
Contributor Author

CurrentGlucoseView

Thanks for looking! That is in another file, right? I don't think that glucoseFormatter will be accessible here. But I suppose my solution is a minimal and acceptable change, isn't it?

@mountrcg
Copy link
Contributor

But I suppose my solution is a minimal and acceptable change, isn't it?

Totally!

As per discussion of targets and rounding errors leading to changed targets after multiple unit switches, I suspect that using

private var glucoseFormatter: NumberFormatter {
would not lead to these. I think @polscm32 already centralized all formatters in Helpers at one point in some branch, which I guess is something we should also do. It only popped into my mind when viewing this PR as there are some inconsistencies in how rT.variables are handed over from oref to swift. Some are "convert_bg functioned" some always in md/dL - I think I am partly responsible for that and started fixing it, so I stumbled across glucoseformatter just this week.

@bjornoleh
Copy link
Contributor Author

@mountrcg , perhaps you can write up an issue about this for later?

Sjoerd-Bo3
Sjoerd-Bo3 previously approved these changes Jul 2, 2024
Copy link
Contributor

@Sjoerd-Bo3 Sjoerd-Bo3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested on watch

@Sjoerd-Bo3 Sjoerd-Bo3 changed the base branch from alpha to dev July 2, 2024 13:43
@Sjoerd-Bo3 Sjoerd-Bo3 dismissed their stale review July 2, 2024 13:43

The base branch was changed.

@Sjoerd-Bo3 Sjoerd-Bo3 self-requested a review July 2, 2024 13:43
Copy link
Contributor

@AndreasStokholm AndreasStokholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tested in sim with iOS 16.4, iOS 17.5, watchOS 10.4

@Sjoerd-Bo3 Sjoerd-Bo3 merged commit a364b62 into nightscout:dev Jul 2, 2024
@MikePlante1 MikePlante1 mentioned this pull request Jul 16, 2024
mountrcg pushed a commit to mountrcg/Trio that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Eventual BG for mmol/L is presented with irrelevant precision (2 decimal places)
4 participants